Relocate License Logic on Windows systems into Ruby #212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
----------
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the
best of my knowledge, is covered under an appropriate open
source license and I have the right under that license to
submit that work with modifications, whether created in whole
or in part by me, under the same open source license (unless
I am permitted to submit under a different license), as
Indicated in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including
all personal information I submit with it, including my
sign-off) is maintained indefinitely and may be redistributed
consistent with this project or the open source license(s)
involved.
Signed-off-by: Jeff Dean <jwdean@scriptpro.com>
phiggins
approved these changes
Sep 21, 2020
|
TravisCI seems to be failing on this repo for about a year now with the same error. So i dont think it's related to this PR. |
phiggins
reviewed
Sep 22, 2020
providers/default.rb
Outdated
Comment on lines
501
to
507
| post_action = if (new_resource.post_install_action == 'exec') && (desired_version.major >= 15) | ||
| "#{new_resource.exec_command} --chef-license #{license_provided}" | ||
| elsif desired_version.major < '15' | ||
| new_resource.exec_command | ||
| else | ||
| '' | ||
| end |
There was a problem hiding this comment.
Suggested change
| post_action = if (new_resource.post_install_action == 'exec') && (desired_version.major >= 15) | |
| "#{new_resource.exec_command} --chef-license #{license_provided}" | |
| elsif desired_version.major < '15' | |
| new_resource.exec_command | |
| else | |
| '' | |
| end | |
| post_action = if new_resource.post_install_action != 'exec' | |
| '' | |
| elsif desired_version.major >= 15 | |
| "#{new_resource.exec_command} --chef-license #{license_provided}" | |
| else | |
| new_resource.exec_command | |
| end |
I think this conditional needs to change to maintain the old behavior.
The whitespace is probably all wrong because I'm trying to do this in github's comment box.
----------
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the
best of my knowledge, is covered under an appropriate open
source license and I have the right under that license to
submit that work with modifications, whether created in whole
or in part by me, under the same open source license (unless
I am permitted to submit under a different license), as
Indicated in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including
all personal information I submit with it, including my
sign-off) is maintained indefinitely and may be redistributed
consistent with this project or the open source license(s)
involved.
Signed-off-by: Jeff Dean <jwdean@scriptpro.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous logic used PowerShell to call ruby.exe, with the ruby being an embedded string within the PowerShell script. That ruby used LicenseAcceptance::Acceptor to apply the license acceptance condition. This also used chef_install_dir to determine where Chef was located.
The new logic eliminates the embedded Ruby by determining if there is license acceptance before starting PowerShell to modify the command line of the post-action ChefClient execution. This now will use the --chef-license command line parameter to affect the license state. Instead of using chef_install_dir to determine where Chef is installed it will use the exec_command resource.
Description
There are two desired impacts of this change
Stylistically it is poor form to embed Ruby in PowerShell that is effectively called by Ruby. If there were tests for this code, it would be untestable.
The use of chef_install_dir is used for two non-congruent purposes in the cookbook:
-- determines the directory where the PowerShell script to be run is located.
chef_client_updater/providers/default.rb
Line 309 in a709307
That location would (by default) be C:\OpsCode.
-- determines where ruby.exe is located (in PowerShell).
chef_client_updater/providers/default.rb
Line 571 in a709307
The need this addresses is to allow the PowerShell script to be run from a directory other than that in which Chef is installed. This is a security requirement for us. While the more obvious change may have been to modify the behavior of the first case above, the, changing the second is a light touch that does should not change the current, expected behavior.
Issues Resolved
None
Check List
I've been so far unsuccessful at setting up testing for community cookbooks outside of my own lab. Working on it still.